Jit preparation for arm64 apple: Use bytes in fgArgTabEntry.
#42503
Conversation
sandreenko
added
the
area-CodeGen-coreclr
sandreenko
force-pushed
the
Arm64Apple-Jit-Part2
branch
6 times, most recently
from
bebb74c to
9422786
Compare
sandreenko
changed the title
fgArgTabEntry.
It will fail on arm64 apple.
sandreenko
force-pushed
the
Arm64Apple-Jit-Part2
branch
from
9422786 to
1aba9e4
Compare
sandreenko
force-pushed
the
Arm64Apple-Jit-Part2
branch
from
1aba9e4 to
1ca1a42
Compare
|
PTAL @dotnet/jit-contrib |
src/coreclr/src/jit/jit.h
Outdated
| @@ -245,6 +245,20 @@ | |||
| #define UNIX_AMD64_ABI_ONLY(x) | |||
| #endif // defined(UNIX_AMD64_ABI) | |||
|
|
|||
| #if defined(DEBUG) && !defined(OSX_ARM64_ABI) | |||
| #define DEBUG_NOT_OSX_ARM64_ABI | |||
There was a problem hiding this comment.
IMO these would be better left as, e.g. defined(DEBUG) && !defined(OSX_ARM64_ABI) at each use. I don't find the implied conjunction clear enough, though I see that it might be difficult to deal with the #defines below. Perhaps you could define something like TRACK_ARG_SLOTS or DEBUG_ARG_SLOTS and have a comment here about why it is not used for the OSX_ARM64_ABIT case?
There was a problem hiding this comment.
I would say it is a temporary solution, all code under DEBUG_NOT_OSX_ARM64_ABI will go away once the work is done and we check that the new byte mechanism is correct and matches the old results. However, I have found it useful to use a new define because now I can easily change it:
- define it in release for non apple arm64;
- replace
assert-s withnoway_assert-s and make debugging easier;
etc.
There was a problem hiding this comment.
Yes, I agree that it's not a huge deal since it's temporary, but I appreciate you making the changes~
sandreenko
force-pushed
the
Arm64Apple-Jit-Part2
branch
from
cdf55db to
e674e5a
Compare
ghost
locked as resolved and limited conversation to collaborators
In order to support Arm64 Apple ABI we need to know the exact size of small arguments because when they are passed on stack they are passed without alignment.
There are two main parts that need changes:
fgArgInfoandGenTreePutArgStk. In this PR I am addingbyteSize/byteOffsettofgArgInfo. I am keeping the old `stackSlot/stackSize(in target_pointer sized slots) in place for now but use it only in asserts under debug. I will delete them after all changes are in and we confirm that both models give us the same results on non affected platforms.No diffs(both jit-diff and spmi).
I recommend reviewing by commits (because the first are just refactoring changes):
d888195: fix a dumping error introduced in my recent ref PR.
5f5e631: add a pinvoke test with many small stack arguments.
It will fail on arm64 apple.
345d355: Add get/set for lclVar::lvStkOffs.
20fdade: Change some types from
size_ttounsignedwhen working with stack offsets.80beef0: Rename
stackSizetoGetStackSize.49ed384: Do not run stack level setter for arm64 apple.
I had a prototype that was supporting byte sizes there but the cost was too high for this PR. The main issue is that we will have to track alignment between the currect argument and the next one and currently it is not working well. For example, the stack level setter ignores arm32 double alignment requirements and gives wrong results there when I fixed it I got asm diffs because of the #42673. The stack level setter does not affect arm64 because there we always save stack frame so it is safe to skip it there.
1ca1a42: Use bytes in
fgArgTabEntry.The main change, I could put it in a separate PR if that will make review easier.